Run all integration test in parallel #1912

amitkrout · 2019-07-16T07:03:35Z

What is the purpose of this change? What does it change?

Runs all command integration test through a single .PHONY statement.

Was the change discussed in an issue?

fixes - part of #1473

How to test changes?

make test-integration-all
make test-integration-all-service-catalog

amitkrout · 2019-07-27T02:10:47Z

✗  Unable to connect to OpenShift cluster, is it down

/test integration

amitkrout · 2019-07-27T04:57:01Z

[odo]  ✗  Waiting for build to finish [30s]
[odo]  ✗  Failed to create component with name cmp-git. Please use `odo config view` to view settings used to create component. Error: unable to wait for build cmp-git-testing-1 to run: timed out waiting for the condition

/test integration

amitkrout · 2019-07-27T07:10:38Z

[odo]  ✗  Unable to connect to OpenShift cluster, is it down?

/test integration

amitkrout · 2019-07-27T09:02:57Z

[odo]  ✗  Waiting for build to finish [30s]
[odo]  ✗  Failed to create component with name nodejs. Please use `odo config view` to view settings used to create component. Error: unable to wait for build nodejs-myapp-1 to run: timed out waiting for the condition

/test integration

amitkrout · 2019-07-28T04:46:19Z

[odo]  ✗  Waiting for build to finish [30s]
[odo]  ✗  Failed to create component with name cmp-git. Please use `odo config view` to view settings used to create component. Error: unable to wait for build cmp-git-testing-1 to run: timed out waiting for the condition

/test integration

amitkrout · 2019-07-29T06:45:42Z

Tested on CI (OpenShift [1] and travis[2]), however found a flake #1955 on OpenShift CI.

[1] https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/1912/pull-ci-openshift-odo-master-integration/1033
[2] https://travis-ci.com/openshift/odo/jobs/220183820

dharmit

Requested few minor changes with the code documentation
At multiple places, I see that project variable's definition is being moved by a few lines. Since it's happening many times in the PR, I'm curious to know it's significance.

dharmit · 2019-07-29T07:57:56Z

Makefile

-.PHONY: test-integration
-test-integration:
-	go test -v github.com/openshift/odo/tests/integration -ginkgo.slowSpecThreshold=$(SLOW_SPEC_THRESHOLD) -ginkgo.v -timeout $(TIMEOUT)
+# Runs all integration test irrespective of service catalog status in the cluster


When we say "Runs all integration tests" it includes every test, isn't it? But link and service catalog tests run through test-integration-all-service-catalog.

yup, i understand.

I am open to better name suggestion (Keep in mind that tests depending on service catalog are not part of OpenShift CI).

I'm not suggesting a name change. I'm suggesting to mention that "link" and "service catalog" tests do not run as a part of this test. 🙂

Done, does it make sense ?

dharmit · 2019-07-29T07:59:58Z

Makefile

+	-focus="odoCmdApp|odoCmpSubE2e|odoCmpE2e|odo config test|odo storage command|odoWatchE2e|odo generic|odoJavaE2e|odojsonoutput|odo push command tests|odoURLIntegration|odoSourceE2e" \
+	slowSpecThreshold=$(SLOW_SPEC_THRESHOLD) -randomizeAllSpecs  tests/integration/ -timeout 7200s
+
+# Run all integration test which are depend on service catalog enabled cluster


In this comment, can we mention the tests that are dependent on service catalog being enabled in cluster? And, if relevant, also mention that in future, tests which require service catalog should be added under this PHONY and not elsewhere?

In this comment, can we mention the tests that are dependent on service catalog being enabled in cluster?

Agree

And, if relevant, also mention that in future, tests which require service catalog should be added under this PHONY and not elsewhere?

This piece of info i am going to add as part of #1831

dharmit · 2019-07-29T08:02:22Z

docs/development.adoc

-* For the entire integration test suite:
+Entire integration tests suite can be run in parallel and sequentially separately:
+
+* To run entire integration tests spec in parallel (default: 2 ginkgo test node), on a test cluster:


Should we also mention how someone can set the value to something other than 2 if it's relevant for their environment?

Sure, i can put a note or a place where it fits better in the doc

dharmit · 2019-07-29T08:03:36Z

docs/development.adoc

 ----
 +

-NOTE: `make test-integration` doesn't honour enviornment variable `TEST_EXEC_NODES`. So by default it runs the entire integration test suite on a single ginkgo test node sequentially.
+NOTE: `make test-integration-all-service-catalog` can be successfully validated only if the cluster has service catalog enabled. Whereas `make test-integration-all` runs the test irrespective of service catalog status in the cluster.


But, IIUC, make test-integration-all doesn't run the tests which explicitly require service catalog to be up. Should we mention that somewhere in this line or elsewhere? Apologies if my understanding is incorrect or it has been mentioned elsewhere already.

Should we mention that somewhere in this line or elsewhere?

I am sorry, could not get you.

Whereas `make test-integration-all` runs the test irrespective of service catalog status in the cluster.

This is the explanation what you are looking for i guess. Right ?

dharmit · 2019-07-29T08:05:01Z

pkg/preference/preference.go

@@ -115,7 +115,7 @@ func NewPreferenceInfo() (*PreferenceInfo, error) {
 	configFile, err := getPreferenceFile()
 	glog.V(4).Infof("The configFile is %+v", configFile)
 	if err != nil {
-		return nil, errors.Wrap(err, "unable to get odo config file")
+		return nil, errors.Wrap(err, "unable to get odo preference file")


Shouldn't we also change https://github.com/openshift/odo/pull/1912/files#diff-5f21db40ee2e66f00f206912f4dddc2fR116 to write preferenceFile instead of configFile? Using configFile and preferenceFile interchangeably could cause ambiguity, no?

+1, I am going to update it in the next commit.

dharmit · 2019-07-29T08:05:48Z

tests/e2escenarios/core_beta_flow.go

 		context = helper.CreateNewContext()
 		os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml"))
+		project = helper.CreateRandProject()


Out of curiosity, what impact does this have whether we create a project earlier or later?

It is just to make sure that the set global preference path is used in the project creation step though there is no validation done for it. But if you keep the project create step as it was then project creation step picks the default preference path (~/.odo/preference.yaml) and then the very next odo command start picking the preference path from newly set path.

So i cleaned up this in all test file and later i will create some fresh spec where the validation will be done including all possible cases without duplicating the scenario.

This is for another discussion, but I think that CreateRandProject should be using oc not odo.

tests/integration/cmd_link_unlink_test.go

kadel · 2019-08-16T08:18:07Z

/lgtm

openshift-ci-robot added the size/S label Jul 16, 2019

openshift-ci-robot requested review from kadel and surajnarwade July 16, 2019 07:03

amitkrout force-pushed the runAllIntegrationTestInParallel branch from 3e1ade5 to d763b3c Compare July 16, 2019 15:02

amitkrout changed the title ~~Run all integration test in parallel~~ [WIP] Run all integration test in parallel Jul 16, 2019

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. size/L and removed size/S labels Jul 16, 2019

amitkrout force-pushed the runAllIntegrationTestInParallel branch 3 times, most recently from a2be939 to 10f57c6 Compare July 24, 2019 06:26

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 25, 2019

amitkrout force-pushed the runAllIntegrationTestInParallel branch from fbc994e to 29967ab Compare July 26, 2019 13:40

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Jul 26, 2019

amitkrout force-pushed the runAllIntegrationTestInParallel branch from 51f0fa9 to a434986 Compare July 26, 2019 18:31

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 26, 2019

amitkrout changed the title ~~[WIP] Run all integration test in parallel~~ Run all integration test in parallel Jul 28, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 28, 2019

dharmit suggested changes Jul 29, 2019

View reviewed changes

amitkrout force-pushed the runAllIntegrationTestInParallel branch 4 times, most recently from c4938ce to 65e2e04 Compare July 31, 2019 14:33

amitkrout added 25 commits August 16, 2019 10:52

Checking how integration all behaves in CI

e9149cc

Updated preference path

0855013

Fixing error

e2f6e5c

Updated preference file as per girish

c2c976e

Fixing test failure

f04e72c

Updated travis file

982a6a7

Fixed test failure

66c1632

Fixed some more test failure

ca0b9ef

Fixed some more test failure

fab19d1

Increasing CI timeout

3f9eaa4

Updating test

78faa87

Separating service catalog tests

0efcd0f

Updated Makefile

c129b97

Fixed context dir

0909b8b

Fixed context dir per spec

25be32f

Updated CI files and doc

499952c

Updated doc and Makefile comment

9202ea9

Review comments are addressed

922d9e2

Updated preference text

4309755

Updated Makefile

6979970

Updated docs

e705819

Updated as per review comment

ebc4639

Updated tests

82e078d

Updated doc

44053e9

Updated Makefile

36fd8f4

amitkrout force-pushed the runAllIntegrationTestInParallel branch from b56eccb to 36fd8f4 Compare August 16, 2019 05:29

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 16, 2019

openshift-merge-robot merged commit baf308b into redhat-developer:master Aug 16, 2019

rm3l added the estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. label Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run all integration test in parallel #1912

Run all integration test in parallel #1912

amitkrout commented Jul 16, 2019 •

edited

Loading

amitkrout commented Jul 27, 2019 •

edited

Loading

amitkrout commented Jul 27, 2019 •

edited

Loading

amitkrout commented Jul 27, 2019

amitkrout commented Jul 27, 2019

amitkrout commented Jul 28, 2019

amitkrout commented Jul 29, 2019 •

edited

Loading

dharmit left a comment

dharmit Jul 29, 2019

amitkrout Jul 29, 2019

dharmit Jul 30, 2019

amitkrout Jul 31, 2019

dharmit Jul 29, 2019

amitkrout Jul 29, 2019

dharmit Jul 29, 2019

amitkrout Jul 29, 2019

amitkrout Jul 30, 2019

dharmit Jul 29, 2019

amitkrout Jul 29, 2019 •

edited

Loading

dharmit Jul 29, 2019

amitkrout Jul 29, 2019

amitkrout Jul 30, 2019

dharmit Jul 29, 2019

amitkrout Jul 29, 2019

kadel Aug 2, 2019

kadel commented Aug 16, 2019

Run all integration test in parallel #1912

Run all integration test in parallel #1912

Conversation

amitkrout commented Jul 16, 2019 • edited Loading

What is the purpose of this change? What does it change?

Was the change discussed in an issue?

How to test changes?

amitkrout commented Jul 27, 2019 • edited Loading

amitkrout commented Jul 27, 2019 • edited Loading

amitkrout commented Jul 27, 2019

amitkrout commented Jul 27, 2019

amitkrout commented Jul 28, 2019

amitkrout commented Jul 29, 2019 • edited Loading

dharmit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitkrout Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Aug 16, 2019

amitkrout commented Jul 16, 2019 •

edited

Loading

amitkrout commented Jul 27, 2019 •

edited

Loading

amitkrout commented Jul 27, 2019 •

edited

Loading

amitkrout commented Jul 29, 2019 •

edited

Loading

amitkrout Jul 29, 2019 •

edited

Loading